Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Log function info in env mapping error messages #14

Merged
merged 1 commit into from
Apr 25, 2017
Merged

Log function info in env mapping error messages #14

merged 1 commit into from
Apr 25, 2017

Conversation

CoryDanielson
Copy link
Collaborator

@CoryDanielson CoryDanielson commented Apr 19, 2017 •
edited
Loading

Problem

When a yielded value is not found in envMapping, the entire val is printed to the console via JSON.stringify, which, by default, does not output a key/value pair when the value is a function. Without information about the function being output in the error message, it's more difficult to track down what the missing mapping is.

Fix

Updated the JSON.stringify call to use a custom replacer that outputs the function name or info about the anonymous function.

Env Mapping is missing a value for {
  "select": "[Function: namedFunction]: function namedFunction() {/* function definition */}"
}

Env Mapping is missing a value for {
  "func": "[Function]: function () {/* function definition */}"
}

@timbuckley timbuckley self-assigned this Apr 19, 2017
@CoryDanielson
Copy link
Collaborator Author

CoryDanielson commented Apr 19, 2017 •
edited
Loading

The use case for this was that I had updated a saga to include additional yield select(...) calls but when I ran my tests, the error message was informing me that I was missing envMapping for

Env Mapping is missing a value for {
  "@@redux-saga/IO": true,
  "SELECT": {
  "args": [
    { /* some huge object */ },
    null
  ]
}

I found it tedious to look at the args and then reference the updated saga to see what yield select those args corresponded to. I actually overrode JSON.stringify globally to do this for me. Then forked and did this PR

After the fix, that output was something like

Env Mapping is missing a value for {
  "@@redux-saga/IO": true,
  "SELECT": {
  "selector": "[Function: nameOfMyReduxSelector]: /* function definition */",        // This line was added
  "args": [
    { /* some huge object */ },
    null
  ]
}

@timbuckley
Copy link
Owner

That makes a lot of sense! I actually had the same issue myself, but wasn't sure of a way to improve the situation. Checking this out now.

Excellent improvement @CoryDanielson!

@CoryDanielson
Copy link
Collaborator Author

CoryDanielson commented Apr 19, 2017 •
edited
Loading

I pushed up a second commit so that the output matches node more. The output is more clear about the values being functions as well.

In node REPL:

> console.log(function test() {})
[Function: test]
> console.log(function() {});
[Function]

I edited my PR comment to include the new output.

Anonymous functions will still have the definition output by fn.toString()

timbuckley
timbuckley reviewed Apr 19, 2017
View reviewed changes
tests/index.js Outdated
t.throws(
() => sagaTestEngine(f3, badMapping3),
`Env Mapping is missing a value for ${JSON.stringify({ func: `[Function]: ${anonymousFunction.toString()}` }, null, 2)}`
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this fails on newer versions of node.

✔ redux-saga-test-engine (e8b2e13...) 🚀  nvm use 6.0.0
Now using node v6.0.0 (npm v3.8.6)
✔ redux-saga-test-engine (e8b2e13...) 🚀  yarn run test
yarn run v0.23.2
$ ava tests/

  10 passed

✨  Done in 0.68s.
✔ redux-saga-test-engine (e8b2e13...) 🚀  nvm use 7.0.0
Now using node v7.0.0 (npm v3.10.8)
✔ redux-saga-test-engine (e8b2e13...) 🚀  yarn run test
yarn run v0.23.2
$ ava tests/

  9 passed
  1 failed

  sagaTestEngine throws under bad conditions
  /Users/tbuckley/dnainfo/redux-saga-test-engine/tests/index.js:190

   189:   const badMapping3 = [['also no', 'functions']]
   190:   t.throws(
   191:     () => sagaTestEngine(f3, badMapping3),

  Env Mapping is missing a value for {
    "func": "anonymousFunction"
  }


error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

The issue seems to be that function expressions like var anonymousFunction = function() { return '...' } get assigned the variable name for their function name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmmm interesting. I noticed that code run through Babel does the same thing. I might have to just remove the test unless I can think of something creative. Maybe conditionally skip the test if a function that is expected to be anonymous gets a name assigned to it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right that we can just remove or water down that particular test. The point is to provide as much information as possible to the user that the node engine allows.

@CoryDanielson
Log function info in env mapping error messages
584cf77 
- Anonymous function is skipped if node names the function.
@CoryDanielson
Copy link
Collaborator Author

Okay, I've pushed up some changes and squashed my previous commits.

The error messages for anonymous and named functions are now consistent. Both will output the [Function: functionName]: <function.toString()> or [Function]: <function.toString()>. It seemed better to have them be consistent in case some users prefer looking at the function definition (especially in cases where they may have identically named functions in their codebase).

I also did a check for the anonymous function .name inside of the unit test. If the function is named, the test is skipped.

@CoryDanielson
Copy link
Collaborator Author

This should be good to run through CircleCI again when you have time.

@timbuckley timbuckley merged commit 18c74d6 into timbuckley:master Apr 25, 2017
@timbuckley
Copy link
Owner

Looks like it passed CI on master. I'll bump the version and deploy to npm soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timbuckley timbuckley timbuckley left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CoryDanielson @timbuckley